-
Notifications
You must be signed in to change notification settings - Fork 1.3k
✨ Add WithWaitForHandlerSync to delay reconciliation until handlers sync #3406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
✨ Add WithWaitForHandlerSync to delay reconciliation until handlers sync #3406
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GonzaloLuminary The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @GonzaloLuminary! |
|
Hi @GonzaloLuminary. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
I thought this already works out of the box as every Get/List call of the cached reader check if the corresponding informer has synced |
We thought the same, but we were not able to get the initial reconciles to happen after all the elements coming from the first List passed through the event handlers. The test added in this PR does not pass in master which indicates that reconciles are happening before all initial elements go through the event handlers. We think that controller-runtime is only waiting for the first List to end before starting the reconcile workers. We'd prefer in some cases for the reconciles to happen after all initial elements have passed through the event handlers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
@GonzaloLuminary please sign the CLA and use an e-mail address connected to your github account so we can accept your contribution
|
|
||
| // WaitForHandlerSync when set to true, waits for the handler registration's HasSynced | ||
| // before starting reconciliation. | ||
| WaitForHandlerSync bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to make this optional? IOW, are there known use-cases where one would not want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option slows down reconciliations after reboots in non-HA mode. Given that business logic is usually only implemented in Reconcile, I think it can be dangerous to always leave this option as true due to its performance implications.
I am guessing that unblocks alrready once the store got synced, but that doesn't necesitate that he handlers did finish processing, especially if they are slow and potentially themselves do something like listing against the cache? And ofc we only do this for the primary object of the controller, not secondary for which it may have handlers. |
6ad796a to
5509dd7
Compare
Adds
WithWaitForHandlerSyncoption tosource.KindWithOptionsto ensure reconciliation does not start until all handlers have synced. This is important for controllers that need a global picture of existing resources before making reconciliation decisions.Examples include the k8s ResourceQuota controller and cluster autoscaler, which do this internally with client-go. This PR brings the same capability to controller-runtime.
Related: Projects like Kueue could also benefit from this feature, though they would need additional API to coordinate handler sync across multiple managers before allowing reconciliation.